Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow event-routing-backends to be used when transforming non-openedx events #431

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Jun 12, 2024

Description:

Event Routing Backends is designed to be installed with edx-platform, and the code assumes this. But we want to use ERB outside of the platform too -- it contains event transform classes and test logic which is useful when transforming non-openedx events.

This PR:

  • Catches ImportErrors from edx-platform to fix errors like these
  • Adds a utility function to add custom events to the backend whitelists.
  • Refactors transformer test mixin classes to make them available for use outside of ERB.

See open-craft/openedx-completion-aggregator#205 for an example usage.

JIRA: FAL-3766 (private-ref)

Part of openedx/openedx-aspects#222

Merge deadline: end of June

Testing instructions:

See open-craft/openedx-completion-aggregator#205 for manual test instructions.

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 12, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 12, 2024

Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

so that event-routing-backends can be dependency for non-edx-platform
repos like openedx-completion-aggregator.
* Pulls the EVENT_TRACKING_BACKENDS configuration template into a
  utility method so it can be used by other apps.
* Pulls allowed xAPI and Caliper events into settings to preserve
  changes
* Adds a test
pomegranited added a commit to open-craft/openedx-completion-aggregator that referenced this pull request Jun 12, 2024
in class variable name and in a comment.
Refactors the transformer test classes to split the test functionality into two:

* TransformersFixturesTestMixin -- for running the event fixture tests
* TransformersTestMixin -- for running the ERB-specific event tests

The fixtures file path constants have been moved to property methods so
they can be overrideen when used outside of ERB.
This exception may occur when using ERB transformer test mixins because
the fixture test files are not installed with this package.
Copy link
Contributor

@Ian2012 Ian2012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our use case, we were copying the mock_helpers function to other packages to be able to extend this. This looks good to me

…tings

Initialize EVENT_TRACKING_BACKENDS_ALLOWED_XAPI_EVENTS and
EVENT_TRACKING_BACKENDS_ALLOWED_CALIPER_EVENTS only if they aren't
already initialized, and append our events to them.

This allows other plugins to modify these settings too.
@pomegranited
Copy link
Contributor Author

Thank you for your review @Ian2012 !

@bmtcril or @ziafazal is there anything else I need to fix here before this can be merged?

Copy link
Contributor

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@bmtcril bmtcril merged commit 9ae9c4f into openedx:master Jun 20, 2024
11 checks passed
@openedx-webhooks
Copy link

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants